Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
+11
to
+15
| printf '%s %s\n' "$schedule" "$cmd" > /tmp/crontab | ||
| exec supercronic /tmp/crontab | ||
| else | ||
| exec go-cron "$schedule" sh -c "$cmd" | ||
| fi |
Contributor
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
Suggested change
| printf '%s %s\n' "$schedule" "$cmd" > /tmp/crontab | |
| exec supercronic /tmp/crontab | |
| else | |
| exec go-cron "$schedule" sh -c "$cmd" | |
| fi | |
| printf '%s %s\n' "$schedule" "$cmd" >/tmp/crontab | |
| exec supercronic /tmp/crontab | |
| else | |
| exec go-cron "$schedule" sh -c "$cmd" | |
| fi |
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector" |
gkech
force-pushed
the
K8SPG-851
branch
6 times, most recently
from
July 21, 2026 11:10
5f0a128 to
7ddc3cd
Compare
hors
reviewed
Jul 21, 2026
| # - name: s3-ca | ||
| # secret: | ||
| # secretName: my-s3-ca | ||
| # logRotate: |
Collaborator
There was a problem hiding this comment.
@gkech Can we use the same naming as in PSMDBO https://github.com/percona/percona-server-mongodb-operator/blob/main/deploy/cr.yaml
And also, please add the possibility of adding custom liveness/readyness probes for two new sidecars (we have added it for PSMDB as well).
Collaborator
There was a problem hiding this comment.
@gkech, please update cr.yaml as well, and make sure we can add custom liveness/readiness probes (if we do not have a default one).
| local logrotate_additional_conf_files=() | ||
| local conf_d_dir="/opt/crunchy/logcollector/logrotate/conf.d" | ||
|
|
||
| # Operator-managed postgres.conf overrides the default when present. |
Contributor
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
Suggested change
| # Operator-managed postgres.conf overrides the default when present. | |
| # Operator-managed postgres.conf overrides the default when present. |
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a new spec.logCollector feature to wire Fluent Bit + logrotate sidecars into PostgreSQL instance pods, including CRD/API updates, operator reconcile logic, container assets in the operator image, and E2E coverage.
Changes:
- Add
LogCollectorSpec/LogRotateSpecto the PerconaPGCluster API and regenerate deepcopy + CRD/bundle manifests. - Implement reconciliation to (a) inject log collector sidecars/volumes and (b) create/update/delete backing ConfigMaps.
- Add build assets (entrypoint + fluent-bit/logrotate configs), default CR example, and a new KUTTL E2E test suite for log collection/rotation/S3 shipping.
Reviewed changes
Copilot reviewed 32 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go | Adds deepcopy support for new log collector API types. |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Adds spec.logCollector API types and helper LogCollectorEnabled(). |
| percona/logcollector/reconcile.go | Implements sidecar/volume wiring and ConfigMap reconciliation for log collector. |
| percona/logcollector/reconcile_test.go | Unit tests for reconcile behavior (ConfigMap lifecycle + sidecar wiring + version gate). |
| percona/logcollector/logrotate/container.go | Builds the logrotate sidecar container spec. |
| percona/logcollector/container.go | Builds fluent-bit + logrotate containers, volumes, and security context defaults. |
| percona/logcollector/container_test.go | Unit tests for constructed containers/volumes. |
| percona/controller/pgcluster/controller.go | Hooks logcollector reconciliation into main PGCluster reconcile flow. |
| internal/naming/names.go | Removes unused PGBackRestRepoName constant. |
| e2e-tests/vars.sh | Adds IMAGE_LOGCOLLECTOR test image variable. |
| e2e-tests/functions | Wires IMAGE_LOGCOLLECTOR into generated CRs for tests. |
| e2e-tests/run-release.csv | Enables logcollection scenario in release E2E runs. |
| e2e-tests/run-pr.csv | Enables logcollection scenario in PR E2E runs. |
| e2e-tests/tests/logcollection/00-assert.yaml | Asserts operator deployment/CRD presence for logcollection scenario. |
| e2e-tests/tests/logcollection/00-deploy-operator.yaml | Deploys operator + client + MinIO prerequisites for logcollection scenario. |
| e2e-tests/tests/logcollection/01-assert.yaml | Asserts cluster readiness and expected ConfigMaps. |
| e2e-tests/tests/logcollection/01-create-cluster.yaml | Creates a cluster with logCollector enabled + custom fluent-bit/logrotate config and MinIO TLS wiring. |
| e2e-tests/tests/logcollection/02-assert.yaml | Asserts backup success used to generate logs. |
| e2e-tests/tests/logcollection/02-generate-logs.yaml | Generates Postgres and pgBackRest logs (SQL + on-demand backup). |
| e2e-tests/tests/logcollection/03-check-collected-logs.yaml | Verifies sidecars exist and logs/config are actually being consumed. |
| e2e-tests/tests/logcollection/04-rotate-logs.yaml | Forces logrotate run inside sidecar. |
| e2e-tests/tests/logcollection/05-check-rotated-logs.yaml | Verifies rotated files exist on the data volume. |
| e2e-tests/tests/logcollection/06-check-s3-logs.yaml | Verifies logs are shipped to MinIO (S3). |
| e2e-tests/tests/logcollection/99-remove-cluster-gracefully.yaml | Cleanup step ensuring no finalizers/panics and operator teardown. |
| deploy/crd.yaml | Updates CRD schema to include spec.logCollector. |
| deploy/bundle.yaml | Updates bundle schema to include spec.logCollector. |
| deploy/cr.yaml | Adds example/default logCollector configuration to sample CR. |
| build/postgres-operator/logcollector/logrotate/logrotate.conf | Provides default logrotate rules for Postgres + pgBackRest logs. |
| build/postgres-operator/logcollector/fluentbit/parsers_multiline.conf | Adds fluent-bit multiline parser configuration. |
| build/postgres-operator/logcollector/fluentbit/fluentbit.conf | Top-level fluent-bit config with includes (including custom override dir). |
| build/postgres-operator/logcollector/fluentbit/fluentbit_pg.conf | Fluent-bit inputs for Postgres and pgBackRest logs + stdout output. |
| build/postgres-operator/logcollector/fluentbit/custom/default.conf | Placeholder custom config file in image layout. |
| build/postgres-operator/logcollector/entrypoint.sh | Sidecar entrypoint: runs fluent-bit or scheduled logrotate with config validation. |
| build/postgres-operator/init-entrypoint.sh | Copies logcollector assets into shared bin volume when present in operator image. |
| build/postgres-operator/Dockerfile | Packages logcollector assets into the operator image at /logcollector. |
Comment on lines
+44
to
+64
| func securityContext(cr *v2.PerconaPGCluster) *corev1.SecurityContext { | ||
| if cr.Spec.LogCollector != nil && cr.Spec.LogCollector.ContainerSecurityContext != nil { | ||
| return cr.Spec.LogCollector.ContainerSecurityContext | ||
| } | ||
|
|
||
| sc := &corev1.SecurityContext{ | ||
| RunAsNonRoot: new(true), | ||
| AllowPrivilegeEscalation: new(false), | ||
| Privileged: new(false), | ||
| Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}, | ||
| SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}, | ||
| } | ||
|
|
||
| // OpenShift assigns one shared UID to every container in the pod. | ||
| if cr.Spec.OpenShift == nil || !*cr.Spec.OpenShift { | ||
| sc.RunAsUser = new(postgresUserID) | ||
| sc.RunAsGroup = new(postgresUserID) | ||
| } | ||
|
|
||
| return sc | ||
| } |
Contributor
Author
There was a problem hiding this comment.
not true, it was with values: https://go.dev/play/p/dTtSaU19JXe
Comment on lines
+53
to
+56
| for i := range cr.Spec.InstanceSets { | ||
| cr.Spec.InstanceSets[i].Sidecars = append(cr.Spec.InstanceSets[i].Sidecars, containers...) | ||
| cr.Spec.InstanceSets[i].SidecarVolumes = append(cr.Spec.InstanceSets[i].SidecarVolumes, volumes...) | ||
| } |
gkech
marked this pull request as ready for review
July 23, 2026 01:03
gkech
requested review from
DhruthiKV,
egegunes,
eleo007,
jvpasinatto,
mayankshah1607,
nmarukovich,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
July 23, 2026 01:03
egegunes
reviewed
Jul 23, 2026
| Path ${PG_LOG_DIR}/*.log,${PG_LOG_DIR}/*.csv | ||
| Tag ${POD_NAMESPACE}.${POD_NAME}.postgres | ||
| Refresh_Interval 5 | ||
| DB /tmp/flb_pg.db |
Contributor
There was a problem hiding this comment.
should we persist this across restarts?
Collaborator
There was a problem hiding this comment.
@gkech Do we need to add ignore_older option in this case? Who will remove this DB if it becomes big? What we will do in case of a major Fluentbit upgrade, which can require a DB update as well. Will we have a real advantage with the persist approach? If yes, we need to change it for all operators.
Contributor
Author
There was a problem hiding this comment.
Do we need to add ignore_older option in this case?
Yes
Contributor
Author
There was a problem hiding this comment.
Who will remove this DB if it becomes big?
if we use ignore_older, the size of the db will be max the amount of days written there, and in general it will not grew thaaat big I guess
| # extra time-based cap. | ||
| /pgdata/pg*/log/*.log | ||
| /pgdata/pg*/log/*.csv { | ||
| daily |
Contributor
There was a problem hiding this comment.
i wonder how this plays with postgresql's daily rotation according to weekday
mayankshah1607
requested changes
Jul 24, 2026
| ExtraConfig corev1.LocalObjectReference `json:"extraConfig,omitempty"` | ||
|
|
||
| // +optional | ||
| Schedule string `json:"schedule,omitempty"` |
Member
There was a problem hiding this comment.
Default schedule can be added via kubebuilder tag
Comment on lines
+907
to
+911
| // +optional | ||
| Configuration string `json:"configuration,omitempty"` | ||
|
|
||
| // +optional | ||
| ExtraConfig corev1.LocalObjectReference `json:"extraConfig,omitempty"` |
Member
There was a problem hiding this comment.
Similar to how we do for PSMDBO, we should add some description so the difference is clear
Suggested change
| // +optional | |
| Configuration string `json:"configuration,omitempty"` | |
| // +optional | |
| ExtraConfig corev1.LocalObjectReference `json:"extraConfig,omitempty"` | |
| // Configuration allows overriding the default logrotate configuration. | |
| // +optional | |
| Configuration string `json:"configuration,omitempty"` | |
| // ExtraConfig allows specifying logrotate configuration file in addition to the main configuration file. | |
| // This should be a reference to a ConfigMap or a Secret in the same namespace. | |
| // Key must contain the .conf extension to be processed correctly. | |
| // +optional | |
| ExtraConfig corev1.LocalObjectReference `json:"extraConfig,omitempty"` |
| return deleteConfigMapIfExists(ctx, c, cr.Namespace, name) | ||
| } | ||
|
|
||
| return createOrUpdateConfigMap(ctx, c, &corev1.ConfigMap{ |
Member
There was a problem hiding this comment.
We are not setting owner references on these ConfigMaps
Comment on lines
+314
to
+316
| if err := logcollector.Reconcile(ctx, r.Client, cr); err != nil { | ||
| return reconcile.Result{}, errors.Wrap(err, "failed to reconcile log collector") | ||
| } |
Member
There was a problem hiding this comment.
I did not test it yet, but if the config is changed, will it take effect into the pods? In PSMDBO we include the logcollector config into the configuration-hash, so restart is triggered
Comment on lines
+29
to
+31
| func postgresLogPath(cr *v2.PerconaPGCluster) string { | ||
| return fmt.Sprintf("/pgdata/pg%d/log", cr.Spec.PostgresVersion) | ||
| } |
Member
There was a problem hiding this comment.
Can the log directory be changed from dynamicConfiguration?
Member
There was a problem hiding this comment.
I see there is a log_directory setting, I don't know if setting it is supported though
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/internal/naming" | ||
| "github.com/percona/percona-postgresql-operator/v2/internal/postgres" | ||
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" | |
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" | |
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" |
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/internal/naming" | ||
| "github.com/percona/percona-postgresql-operator/v2/internal/postgres" | ||
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" | |
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" | |
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" |
| "fmt" | ||
| "reflect" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/internal/naming" | ||
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" | |
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" | |
| pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" |
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" |
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/internal/naming" | ||
| "github.com/percona/percona-postgresql-operator/v2/percona/version" |
Contributor
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
Suggested change
| "github.com/percona/percona-postgresql-operator/v2/percona/version" | |
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector/logrotate" | |
| "github.com/percona/percona-postgresql-operator/v2/percona/version" |
Comment on lines
+742
to
+747
| # # Extra fluent-bit configuration merged with the operator-managed config. | ||
| # configuration: | | ||
| # [FILTER] | ||
| # Name record_modifier | ||
| # Match * | ||
| # Record cluster_name cluster1 |
Comment on lines
+5
to
+20
| - script: |- | ||
| set -o errexit | ||
| set -o xtrace | ||
|
|
||
| source ../../functions | ||
|
|
||
| access_key="$(kubectl -n "${NAMESPACE}" get secret minio-secret -o jsonpath='{.data.AWS_ACCESS_KEY_ID}' | base64 -d)" | ||
| secret_key="$(kubectl -n "${NAMESPACE}" get secret minio-secret -o jsonpath='{.data.AWS_SECRET_ACCESS_KEY}' | base64 -d)" | ||
|
|
||
| check_s3_logs() { | ||
| kubectl -n "${NAMESPACE}" run -i --rm "aws-cli-${RANDOM}" \ | ||
| --image=perconalab/awscli --restart=Never -- \ | ||
| bash -c "AWS_ACCESS_KEY_ID='${access_key}' AWS_SECRET_ACCESS_KEY='${secret_key}' AWS_DEFAULT_REGION=us-east-1 \ | ||
| /usr/bin/aws --endpoint-url https://minio-service:9000 --no-verify-ssl s3 ls s3://operator-testing/ --recursive" \ | ||
| | grep -q postgres | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGE DESCRIPTION
Jira: https://perconadev.atlassian.net/browse/K8SPG-851
Helm PR: TBD
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
FluentBit example configuration used to s3 otel envelope logs to s3 compatible storage:
result in logs in minio(s3 compatible):
more logs:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability